Skip to content

module: add module.customConditions #57688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

module: add module.customConditions #57688

wants to merge 1 commit into from

Conversation

sapphi-red
Copy link
Contributor

Add module.customConditions which exposes the custom resolution conditions specified by the user.

Fixes: #55824

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Mar 30, 2025
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 2 times, most recently from 9efaba7 to 6f23b07 Compare March 30, 2025 08:55
@codecov Codecov
Copy link

codecov bot commented Mar 30, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.86%. Comparing base (aad9030) to head (d576f5c).
⚠️ Report is 274 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57688      +/-   ##
==========================================
- Coverage   89.96%   89.86%   -0.11%     
==========================================
  Files         640      657      +17     
  Lines      188454   193287    +4833     
  Branches    36892    37921    +1029     
==========================================
+ Hits       169546   173698    +4152     
- Misses      11608    12110     +502     
- Partials     7300     7479     +179
Files with missing lines Coverage Δ
lib/module.js 100.00% <100.00%> (ø)

... and 210 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

targos
targos reviewed Mar 31, 2025
View reviewed changes
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch from 6f23b07 to 6bbb2e1 Compare April 1, 2025 11:18
@ljharb ljharb added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 1, 2025
JakobJingleheimer
JakobJingleheimer reviewed Apr 1, 2025
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in order to address the primordial issue, you need to use SafeSet within getUserConditions, and then within the getter, wrap the output of getUserConditions in Set.

@ljharb ljharb mentioned this pull request Apr 1, 2025
Closed
ljharb added a commit to ljharb/node that referenced this pull request Apr 1, 2025
@ljharb
lib: add SafeSet.prototype.toUnsafeSet
92a68f2 
This should help with nodejs#57688 and others
@ljharb
Copy link
Member

ljharb commented Apr 1, 2025

I put up #57723 as a potential way to easily convert a SafeSet to a user-exposable Set, which might help this PR.

ljharb added a commit to ljharb/node that referenced this pull request Apr 1, 2025
@ljharb
lib: add SafeSet.prototype.toUnsafeSet
fe02c30 
This should help with nodejs#57688 and others
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch from 6bbb2e1 to cae9e94 Compare April 3, 2025 14:19
@sapphi-red
Copy link
Contributor Author

Thanks for the reviews!

sapphi-red
sapphi-red commented Apr 3, 2025
View reviewed changes
aduh95
aduh95 reviewed Apr 3, 2025
View reviewed changes
aduh95
aduh95 reviewed Apr 3, 2025
View reviewed changes
aduh95
aduh95 reviewed Apr 3, 2025
View reviewed changes
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 3 times, most recently from 66cc00b to 0789c21 Compare April 6, 2025 12:27
@sapphi-red
Copy link
Contributor Author

Thanks, I've updated the code!

ljharb
ljharb approved these changes Apr 7, 2025
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would i polyfill this in a node that doesn’t have it? Would i have to trawl through NODE_OPTIONS and process.argv and the config file, or is there an easier way?

@JakobJingleheimer JakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@sapphi-red
Copy link
Contributor Author

How would i polyfill this in a node that doesn’t have it? Would i have to trawl through NODE_OPTIONS and process.argv and the config file, or is there an easier way?

As far as I know, that’s the only way.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 21, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/67590/

jasnell
jasnell reviewed Jun 21, 2025
View reviewed changes
jasnell
jasnell reviewed Jun 21, 2025
View reviewed changes
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 2 times, most recently from cac31a9 to 0986bde Compare June 23, 2025 04:03
jasnell
jasnell reviewed Jun 24, 2025
View reviewed changes
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 2 times, most recently from be5a488 to 05d07df Compare June 25, 2025 03:05
jasnell
jasnell reviewed Jun 29, 2025
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/67731/

jsumners-nr
jsumners-nr reviewed Jun 30, 2025
View reviewed changes
Copy link

@jsumners-nr jsumners-nr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think https://github.com/sapphi-red/node/blob/05d07dfb01ad34bfe1d236505b627d6f4465fd74/doc/api/cli.md?plain=1#L477-L502 will need to be updated.

I have never heard of this CLI option before, and neither the docs in this PR nor the docs I have linked make things clear to me.

@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 2 times, most recently from 861529e to e87d6f5 Compare July 4, 2025 04:29
@sapphi-red sapphi-red requested review from Trott, targos and aduh95 July 23, 2025 04:05
ljharb
ljharb reviewed Aug 13, 2025
View reviewed changes
@sapphi-red
module: add module.customConditions
d576f5c 
Add module.customConditions which exposes the
custom resolution conditions specified by the
user.

Fixes: nodejs#55824
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch from e87d6f5 to d576f5c Compare August 19, 2025 07:54
@sapphi-red sapphi-red requested a review from ljharb August 19, 2025 07:55
guybedford
guybedford reviewed Sep 2, 2025
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a really useful feature to have, and I would happily approve progress here.

One problem we have when interacting with loaders though is that the custom conditions of the outer context and inner context might be different when using VM or future loader hook constructs.

The way to resolve this future conflict would be to put this on import.meta instead of as a property - import.meta.customConditions. The justification being the same as for import.meta.resolve() effectively. The process on that would require its own approval going through https://github.com/WinterTC55/import-meta-registry though, which may be harder than this PR. I'm on the fence on this requirement, but it's worth discussing while we are working this through.

My preference would also be to expose this as a frozen array rather than a mutable set to ensure it is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guybedford guybedford guybedford approved these changes

@Trott Trott Awaiting requested review from Trott

@targos targos Awaiting requested review from targos

@aduh95 aduh95 Awaiting requested review from aduh95

@jasnell jasnell Awaiting requested review from jasnell

@JakobJingleheimer JakobJingleheimer Awaiting requested review from JakobJingleheimer

@marco-ippolito marco-ippolito Awaiting requested review from marco-ippolito

@ljharb ljharb Awaiting requested review from ljharb

+1 more reviewer

@jsumners-nr jsumners-nr jsumners-nr approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a way to get the enabled conditions